Re-land the --shadow-tight retirement and close three design-token debt rows - #1942
Conversation
PR #1803 retired the --shadow-tight role alias in favour of the --e1 elevation tier across 49 files and squash-merged as 9d8370a on 2026-08-10. The acf78bf merge on 2026-08-11 silently reverted it, along with six other PRs. This re-applies the retirement against current main: 130 call sites across 67 files, plus both declarations. The alias was a pure pass-through -- `--shadow-tight: var(--e1)` in the light and dark role blocks -- so the substitution is value-preserving. Confirmed for forced-colors too rather than assumed: the `@media (forced-colors: active)` block scopes `:root, .dark`, the same `html` element the alias is declared on, so `--shadow-tight` already resolved through the flattened `--e1: none` there. The .ckb-v2 redeclaration hazard does not bite for the same reason -- .ckb-v2 sits on <html> and .ckb-v2.ckb-v2 outspecifies :root, so both spellings substitute against the winning v2 tier. Two comments survived acf78bf while the code they describe did not: the globals.css note that "the resting-hairline role is gone", and the token test's "unlike the --shadow-tight assertion above". Both are accurate again. The token contract test now sweeps the tracked src tree for both spellings (declaration and var() consumer) instead of only asserting the declaration. A declaration-only check would have caught this particular revert, but only because the declarations happened to come back with the call sites; sweeping both makes the gate independent of which half of a bad merge lands. Mutation-verified in both directions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XsBPUoxpsvTwvFMJzrNoyz
…values `scripts/design-system-contract-baseline.json` is a ceiling, so paying debt down leaves silent headroom behind. Ledger #302 records that pattern: legacyShadowAliases was pinned at 220 against a measured 193, 27 units of unguarded slack, up from 3 units on 2026-08-10. With the previous commit's --shadow-tight retirement applied the gap is wider still -- 220 pinned against 119 measured -- because the reland pays down the debt the acf78bf revert had re-hidden. Four other ratchets had accumulated slack from unrelated work in the same window. legacyShadowAliases 220 -> 119 edgeOwnershipConflicts 27 -> 25 rawPaddingLiterals 67 -> 63 rawGapLiterals 34 -> 32 layoutTransitionExceptions 12 -> 11 Regenerated with --print-debt-baseline rather than hand-edited, so the per-path debtByPath counts move with the totals -- those are what findDebtPathRegressions compares, and the retirement moved them wholesale. Every metric in the diff decreases; nothing is absorbed upward. This is not the baseline refresh #262 warns against. That stop rule forbids refreshing to hide the movement; this pins the movement in so it cannot silently drift back a second time. Mutation-verified: reintroducing one alias in button.tsx now fails at both the total (119 -> 120) and the per-path level. Under the old 220 ceiling the same addition passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XsBPUoxpsvTwvFMJzrNoyz
The active-filter badge sized itself with a raw `h-[1.0625rem] min-w-[1.0625rem]` pair. Ledger #275 tracks that value as leaked debt: it had reached five files, so the fix has always been to tokenise once rather than edit a call site. Re-measured on merged main, the badge role is down to a single call site. #170's convergence landed in the meantime -- document-search- results.tsx now renders the shared control and therapy-compass/ filter-sheet.tsx was deleted outright -- so the leak this row was written about has already been reabsorbed by the extraction. Holding the value in @theme is what stops it leaving again. Two arbitrary values in the same component are deliberately left raw: pr-[0.6875rem] and min-[414px]:max-[429px] -- the repo defines no --breakpoint-* tokens at all, and eight peer sites use the same raw min-[]/max-[] form (359px, 389px, 414px). Naming one window while the peers stay raw is the same drift #275 warns about on another axis, and Tailwind named breakpoints would add variants across the whole utility surface. That belongs in a repo-wide decision, filed separately. The three remaining 1.0625rem hits in mode-nav.tsx and nav-slot-ink.tsx are NOT this token. They size <Icon> glyphs -- a 17px icon against a 12/14/16/20/24 --spacing-icon-* scale -- so folding them under a badge token would merge two roles that only happen to share a number. check:icon-scale deliberately does not flag arbitrary h-[Nrem], so they are a real but separate finding, filed rather than guessed at. The token is also registered in CLINICAL_TWMERGE_THEME.spacing, which tests/tailwind-merge-config.test.ts asserts against the @theme block -- without it `cn()` cannot resolve a conflict on the new utility. Safe by that file's own `tap` reasoning: the single call site is a static string carrying no competing h-*/min-w-* class and never passes through `cn()`, so there is no same-variant pair for declaration to hand to the later class. The entry is protective for future use, not load-bearing today. Value-preserving, and proven rather than inferred: compiling globals.css through @tailwindcss/postcss emits .h-search-band-badge { height: var(--spacing-search-band-badge) } .min-w-search-band-badge { min-width: var(--spacing-search-band-badge) } No ratchet moved, so the ceilings pinned in the previous commit still sit at zero slack. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XsBPUoxpsvTwvFMJzrNoyz
… rows Track A3 is `#262`. Its three parts are now all settled, each checked against code rather than against the row that describes it. Part 1 is the --shadow-tight retirement re-landed earlier in this PR. Part 3 shipped in PR #1780 per `#301`: rawPaddingLiterals, rawRadiusLiterals and rawLineHeightLiterals are live baseline keys enforced over both the class and CSS-declaration spellings, plus rawGapLiterals beyond the original ask. Part 2 needs no work, and that had already been adjudicated -- GATES.md section 3 records it, which is why nothing here builds it. The decidable half of step selection shipped on 9 Aug inside check:design-system- contract: a declared @theme step no production surface selects fails the build. The remaining half -- which existing step a component picks -- is documented there as something "nothing mechanical can" gate, being a judgement about the rendered design rather than a property of the source, with a standing instruction not to duplicate the arbitrary-value check check:type-scale already ships. Reading `#262` alone would have sent a session to build it; that is the `#301` failure mode, so the closure record says so explicitly. Section 3's live status rows carried numbers this PR moved. `#301`'s lesson is that a row understating shipped work is a duplicate-work generator, so they are corrected in the same change: legacyShadowAliases 224 -> 119, and the alias is now retired outright rather than "224 left to retire" edgeOwnershipConflicts 27 -> 25 rawPaddingLiterals 67 -> 63 rawGapLiterals 34 -> 32 layoutTransitionExceptions 12 -> 11 Section 5 is left alone deliberately: it is a dated record measured against 8db1e53, not a live status surface, and rewriting its figures would destroy the provenance it exists to hold. Ledger records are queued as immutable inbox requests: `#262`, `#302` and `#275` closed; two carve-outs split out of `#275` filed as their own rows (the repo-wide breakpoint-token decision, and three 17px mode-nav icon glyphs that sit off the --spacing-icon-* scale with no gate covering them). The queued re-land request 210e3db5 is cancelled rather than reconciled -- its headline "67 files on main still use the retired alias" is false as of this branch, so it would open a row wrong on arrival. The request file and the cancellation both survive as provenance for the acf78bf merge loss. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XsBPUoxpsvTwvFMJzrNoyz
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 7 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (85)
Comment |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XsBPUoxpsvTwvFMJzrNoyz
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f94fa299a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex resolve actionable Codex review findings for this pull request and current head using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. The only repository destination is BigSimmo/Database, and the only branch destination is the pull request head branch claude/design-token-relands at starting commit 50cf53d; never publish fixes to a detached or synthetic work branch and never create a stacked pull request. Use the authenticated GitHub connector to commit each approved fix to BigSimmo/Database:claude/design-token-relands, then verify that the pull request head contains the published commit before reporting success. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. For a fixed thread, reply with as the first line and as the second line. For a no-code disposition, use followed by . These result markers authorize the workflow to close that exact thread only after it verifies a fixed commit is the pull request head; a local-only commit is not a fix. If publication or verification fails, do not use either result marker, do not claim success, and leave the thread open with the blocker. If human input or new authorization is required, do the same. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation. |
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #10766 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
…flight CI failed `docs:check-links` on this branch with Error: cancel request 2e791c01... targets missing pending request 210e3db5... `check-docs-links.mjs` replays the inbox batch to resolve link targets, so an unresolvable request fails it. The cause was a race, not a bad record: PR #1936 reconciled 75 queued requests -- 210e3db5 among them -- while this branch was already in flight. Reconciling moves the request file into `docs/outstanding-issues-inbox/applied/` and allocates it a canonical row, so by the time this branch merged main there was no pending request left for the cancellation to name. Cancelling was the right call against a pending request and is the wrong one against a reconciled row. The cancel is dropped and replaced with a `done` against `#319`, the row 210e3db5 became. That is also the better record: the work is finished rather than withdrawn, so the ledger should carry its outcome and its guard, which a cancellation would have thrown away. Also merges origin/main (this branch was 3 behind) and files two findings the PR preflight surfaced, both deliberately not fixed here: - `check:medication-lexicon-report` has been failing on main for every local `verify:pr-local`, and no CI job runs it -- a grep over .github/workflows finds nothing. It is the last step of the local chain, so it fails preflights while CI stays green. The stale file is a clinical-facing generated document; regenerating it inside a CSS-token PR would bundle a clinical-risk artefact with unrelated chores. - Claude Code web containers can ship Node 22 with no node_modules, which fails `npm ci` on engine-strict before any repo script can run. Re-verified after the merge: the tracked tree still holds zero `--shadow-tight` references, and every pinned ratchet still measures exactly its baseline, so the merge moved no metric and the pins stay honest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XsBPUoxpsvTwvFMJzrNoyz
Summary
Testing
|
Summary
Four low-risk design-token items from the outstanding-issues sweep, bundled because each is a pure CSS/class change with no logic, no clinical or RAG path, and no build-tooling config. Each is its own separately revertible commit. Two follow-up commits repair a ledger request that a concurrent merge invalidated, and retire the token from the three design docs that still described it as live.
Re-land the
--shadow-tightretirement onto--e1(row#319). PR Retire the --shadow-tight role alias onto the --e1 elevation tier #1803 retired this role alias across 49 files and squash-merged as9d8370aon 2026-08-10; theacf78bfmerge on 2026-08-11 silently reverted it along with six other PRs. Re-applied against current main: 130 call sites across 67 files, plus both declarations. The alias was a pure pass-through (--shadow-tight: var(--e1)in both themes), and the@media (forced-colors: active)block scopes:root, .dark— the samehtmlelement the alias is declared on — so it already resolved through the flattened--e1: none. Value-preserving in light, dark and forced-colors.tests/design-token-contract.test.tsnow sweeps the trackedsrctree for both spellings (declaration andvar()consumer) rather than asserting the declaration alone, so the gate no longer depends on which half of a bad merge lands. Mutation-verified in both directions.Re-pin five design-system contract ratchets to their measured values (
#302). The baseline is a ceiling, so paying debt down leaves silent headroom. With the retirement applied:legacyShadowAliases220 → 119,edgeOwnershipConflicts27 → 25,rawPaddingLiterals67 → 63,rawGapLiterals34 → 32,layoutTransitionExceptions12 → 11. Regenerated with--print-debt-baselineso per-pathdebtByPathmoves with the totals — those are whatfindDebtPathRegressionscompares. Every metric in the diff decreases; nothing is absorbed upward. Mutation-verified: reintroducing one alias now fails at both the total and the per-path level, where the old ceiling passed it silently.Hold the search-band count bubble in a spacing token (
#275).1.0625rembecomes--spacing-search-band-badge, consumed ash-search-band-badge/min-w-search-band-badge. Re-measured on merged main the badge role is down from this row's five files to one, because#170's convergence landed in between. Value-preserving and proven rather than inferred: compilingglobals.cssthrough@tailwindcss/postcssemits.h-search-band-badge { height: var(--spacing-search-band-badge) }and the matchingmin-widthrule. Two arbitrary values in the same component are deliberately left raw and re-filed as their own ledger rows — themin-[414px]:max-[429px]window (the repo defines zero--breakpoint-*tokens and eight peer sites use the same raw form, so naming one is the same drift on another axis), and the three1.0625remhits inmode-nav/nav-slot-ink, which size<Icon>glyphs against a 12/14/16/20/24 icon scale and are a different role, not this token.Close out DS Track A3 (
#262parts 2 and 3). Part 3 already shipped in PR feat(design-system): ratchet raw scale literals and gate type-step selection (#262 parts 2 and 3) #1780 per#301; confirmed against the checker, not the row. Part 2 needs no work, and that was already adjudicated indocs/design-system/GATES.md§3: the decidable half of step selection ships insidecheck:design-system-contract, and the remaining half — which existing step a component picks — is documented there as something "nothing mechanical can" gate, with a standing instruction not to duplicate the arbitrary-value checkcheck:type-scalealready ships. §3's live status rows carried the five numbers this PR moved and are corrected in the same commit, per#301's lesson that a row understating shipped work is a duplicate-work generator. §5 is left alone deliberately — it is a dated record, not a live status surface.Retarget the reland ledger record after a mid-flight reconciliation. The first push queued a cancel against inbox request
210e3db5, which was correct at the time. PR chore(issues): reconcile 75 queued ledger requests #1936 then reconciled 75 queued requests —210e3db5among them — moving it todocs/outstanding-issues-inbox/applied/and allocating it canonical row#319.check-docs-links.mjsreplays the inbox batch to resolve link targets, so the now-dangling cancel failed CI withcancel request 2e791c01… targets missing pending request 210e3db5…. The cancel is dropped and replaced with adoneagainst#319, which is also the better record: the work is finished rather than withdrawn, so the ledger keeps its outcome and its guard instead of discarding them.Retire the token from the three remaining design docs.
docs/redesign/02-design-direction.md,docs/redesign/permanent-colour-direction.mdand.design-sync/conventions.mdeach still listed--shadow-tight → --e1as a live alias. The source retirement above had left them stating the opposite of the code. Re-audited afterwards: every surviving mention is deliberate — theGATES.mdprohibition row (naming the banned token is the point), the retirement test, unrelated CLI-flag fixtures inrepo-hygiene.test.ts, the supersededHANDOVER-2026-08-07.md, and a dated historical entry atprocess-hardening.md:454describing a 2026-07-02 change, which would be falsified by editing.Verification
npm run verify:pr-localRe-run after merging
origin/main. All stages pass except one pre-existing failure unrelated to this diff, detailed below.check:medication-lexicon-reportreportsdocs/medication-interaction-lexicon-review.mdis stale. This is pre-existing onorigin/mainand not caused by this branch: the diff touches zero medication, lexicon ordata/files, and a clean worktree at pristineorigin/mainreproduces it at bothd47aa6dand79b01b3. It is also not wired into any CI workflow — a grep over.github/workflows/finds nothing — so it fails every local preflight while CI stays green. Filed as its own ledger row rather than fixed here: the stale file is a clinical-facing generated document, and regenerating it inside a CSS-token PR would bundle a clinical-risk artefact with unrelated chores.Full unit suite:
Token contract and tailwind-merge config gates:
Re-verified on the current tip after both the
origin/mainmerge and the doc commits, rather than assumed: the tracked tree holds zero--shadow-tightreferences insrc/, and every pinned ratchet still measures exactly its baseline — so neither the merge nor the doc changes moved a metric, and the pins stay honest.UI verification not run locally: the change is value-preserving by construction and proven so at the CSS level rather than visually.
--shadow-tightresolved tovar(--e1)in every scope, the badge token compiles to the same1.0625rem, and no measurement moved. There is no rendered difference for a browser gate to detect. CI covers it regardless —Production UI (1),(2),(3),Advisory UIandLighthouse budgetall pass on this head.Risk and rollout
git revertany single commit independently while the branch is open. After the squash-merge the commits fold into one, so a post-merge rollback of one item means reverting that commit's hunks by hand. The baseline re-pin is the only commit with an ordering dependency: it must not be reverted while the retirement stands, or the ratchet regains roughly 100 units of headroom.SKIP_LEDGER_WRITE_GUARD=1escape hatch with the maintainer's approval. The removal is invisible in the merged result — the file appears nowhere ingit diff origin/main...HEAD— and CI's own copy of the same check, which uses the main base, passes. There was no in-place repair available: the tooling forbids cancelling a cancellation by design, and a revert commit registers as the same deletion.Clinical Governance Preflight
classifyPullRequestFilesreturnsclinicalRisk: truefor this diff because it touchesDocumentViewer,document-viewer/**and clinical-dashboard source-rendering components. Those edits are shadow-token substitutions insideclassNamestrings only — no source, citation, retrieval or document-access behaviour changes.ragRankingisfalse; no ranking surface is touched, so noRAG impact:line applies.Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
#319,#262,#302and#275closed, and four new rows filed — the two#275carve-outs (the repo-wide breakpoint-token decision, and three off-icon-scalemode-navglyphs), plus the two findings this preflight surfaced (the stale lexicon report that no CI job runs, and Claude Code web containers shipping Node 22 with nonode_modules, which failsnpm cionengine-strictbefore any repo script can run). Runnpm run issues:reconcileafter this lands.globals.cssnote that "the resting-hairline role is gone" and the token test's "unlike the--shadow-tightassertion above" both survivedacf78bfwhile the code they describe was reverted. Both are accurate again.#262part 2: a raw grep fortext-<step>overcounts, because it also matches the--text-*:declarations and doc comments. That is the 733-vs-705 discrepancyGATES.mdline 66 already warns about, and it reproduces today — a naive sweep returns 773. Use the AST class-root pass.